Skip to content
This repository has been archived by the owner on Jul 21, 2023. It is now read-only.

fix: match libp2p/interface-peer-discovery spec #141

Closed
wants to merge 1 commit into from
Closed

fix: match libp2p/interface-peer-discovery spec #141

wants to merge 1 commit into from

Conversation

andycodesstuff
Copy link

The following pull requests fixes #44 and #111 by matching the libp2p/interface-peer-discovery spec.

vasco-santos
vasco-santos reviewed Aug 24, 2019
View reviewed changes
Copy link
Member

@vasco-santos vasco-santos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @andrycodestuffs

Thanks for your work on this. I would really like to move this forward!

There are some design differences from this implementation to the other that follow the interface-peer-discovery. In this way, you need to work on some other stuff in order to get it compliant:

  • random-walk should be able to be used outside the DHT, the same way as the other peer-discovery implementations README.md#creating-your-own-libp2p-bundle, instead of being started in this module
  • when a peer is discovered, an event needs to be emitted (this is currently happening inside the query, which should not happen)

Thanks for your help, I would suggest that you look at libp2p/js-libp2p-mdns as a ground work. The main difference is that in this case we need to provide the dht instance.

@andycodesstuff
Copy link
Author

Hope to finish this as soon as possible, I'll keep you updated!

@vasco-santos
Copy link
Member

Cool, really glad that you are taking this. Bear in mind that we are also working on moving from callback into Promises, which may reflect on this: ipfs/js-ipfs#1670

@andycodesstuff
Copy link
Author

andycodesstuff commented Aug 25, 2019 •
edited
Loading

@vasco-santos could you please provide a very small example of how to use random walk discovery at the moment?
I'd like to have a reference point to reconstruct the current execution flow

@vasco-santos
Copy link
Member

At the moment, random-walk is started and stopped within the DHT codebase, if it is enabled through the received options:

https://github.com/libp2p/js-libp2p-kad-dht/blob/master/src/index.js#L170
https://github.com/libp2p/js-libp2p-kad-dht/blob/master/src/index.js#L184

Briefly, the random-walk receives the dht instance (https://github.com/libp2p/js-libp2p-kad-dht/blob/master/src/index.js#L136) and uses it to create queries in the DHT.

When a peer is found via those queries, a event is fired

js-libp2p-kad-dht/src/index.js

Line 699 in 30fd44a

this.emit('peer', peerInfo)
with the discovered peer, which should be inside the random-walk scope.

As a result, we should be able to change the configuration to use it separately. Taking an example on the libp2p configuration in js-ipfs (https://github.com/ipfs/js-ipfs/blob/master/src/core/runtime/libp2p-nodejs.js#L43-L48), we should be able to add a random-walk instance inside peerDiscovery. Some stuff needs to change in the configuration side of things too, as we need to create the DHT instance in advance, in order to create the random-walk.

Also note that the random-walk needs to be exported to be used in the js-libp2p scope

@achingbrain
Copy link
Member

Thanks for taking the time to open this, I'm sorry it never landed. Random walk has been removed from the implementation as it can make queries now instead.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@vasco-santos vasco-santos vasco-santos left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

randomWalk should be tested through interface-peer-discovery tests
3 participants
@andycodesstuff @vasco-santos @achingbrain